Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: linting fixes #261

Merged
merged 6 commits into from
Dec 3, 2024
Merged

chore: linting fixes #261

merged 6 commits into from
Dec 3, 2024

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Nov 28, 2024

Description

This pull request implements several important TypeScript and linting improvements to enhance code quality and developer experience. Key changes include:

  • Added .tsx extension to ESLint configuration to ensure proper linting of React TypeScript components
  • Configured TypeScript path alias for @metamask/design-system-react to enable direct imports and proper IDE support
  • Enforced import type syntax through ESLint to prevent runtime errors in .mjs files
  • Updated TypeScript configuration to include the stories folder and its files for design-tokens
  • Ran prettier to apply consistent formatting across all affected files

These changes collectively improve code maintainability, type safety, and development workflow efficiency.

Related issues

Fixes: N/A

Manual testing steps

  1. Run yarn lint and ensure there are no TypeScript linting errors
  2. Run yarn prettier . to verify that all files adhere to the formatting rules

Pre-merge author checklist

Pre-merge reviewer checklist

  • Manually tested the PR to confirm no regressions
  • Verified all linting and formatting changes
  • Confirmed that the updates improve code maintainability

@@ -3,10 +3,10 @@ import type { Meta, StoryObj } from '@storybook/react';
import { Text, TextColor } from '@metamask/design-system-react';
import README from './Shadows.mdx';

interface ShadowSwatchProps {
type ShadowSwatchProps = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use type instead of interface

@@ -2,7 +2,7 @@ import React, { FunctionComponent } from 'react';
import { lightTheme } from '../../../src';
import { Text, TextVariant } from '@metamask/design-system-react';

interface ColorSwatchProps {
type ColorSwatchProps = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use type instead of interface

import { Text, TextVariant } from '@metamask/design-system-react';

interface ColorSwatchGroupProps {
type ColorSwatchGroupProps = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use type instead of interface

Comment on lines 6 to 7
"jsx": "react",
"types": ["react", "node", "jest"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs jsx and react for stories

Comment on lines +12 to +16
"stories/**/*.tsx",
"stories/**/*.ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Includes stories folder in typescript checks but does not include them in build because we are excluding storybook files in tsconfig.packages.build.json

@georgewrmarshall georgewrmarshall marked this pull request as ready for review November 29, 2024 08:01
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner November 29, 2024 08:01
.eslintrc.js Outdated
Comment on lines 39 to 47
'@typescript-eslint/consistent-type-imports': [
'error',
{
prefer: 'type-imports',
disallowTypeAnnotations: true,
fixStyle: 'separate-type-imports',
},
],
'@typescript-eslint/no-import-type-side-effects': 'error',
Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ESLint rule enforces using import type syntax for type imports, which helps prevent runtime errors in .mjs files where type imports aren't supported. It ensures type imports are properly separated from value imports, making the code more maintainable and compatible with ECMAScript modules.

Before

Accidentally included types in mjs files and there is no linting rule preventing this

Screenshot 2024-12-02 at 4 43 10 PM

After

Screenshot 2024-12-02 at 4 40 41 PM

Linting rule

Screenshot 2024-12-02 at 9 01 49 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"lint:dependencies": "depcheck && yarn dedupe --check",
"lint:dependencies:fix": "depcheck && yarn dedupe",
"lint:eslint": "eslint . --cache --ext js,cjs,mjs,ts",
"lint:fix": "yarn lint:eslint --fix && yarn lint:misc --write && yarn constraints --fix && yarn lint:dependencies:fix",
"lint:eslint": "eslint . --cache --ext js,cjs,mjs,ts,tsx",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added .tsx extension to ESLint configuration to ensure React TypeScript components are properly linted. This helps maintain consistent code quality across both JavaScript and TypeScript React and React Native files in our codebase.

Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing example test Button component from design-system-react as it's no longer needed and will be cleaner to add a new Button component

All instances of Button removed

Screenshot 2024-12-02 at 9 04 28 PM

@@ -1,6 +1,7 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes for Text stories now we are including tsx files

type="button"
onClick={() => {
// eslint-disable-next-line no-alert, no-restricted-globals
alert('button-clicked');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously using storybook-actions but once we add it as a dev dependency it clashes with the @storybook/addon-actions version we can fix it in a subsequent PR

@@ -1,8 +1,10 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -52,6 +52,7 @@
"devDependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding missing dev dependency

@@ -1,13 +1,14 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -1,16 +1,16 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -1,26 +1,27 @@
/* eslint-disable no-restricted-globals */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes and ignoring no-restriced-globals so we can get the CSS variables from the stylesheet for typography CSS variables

@@ -1,4 +1,5 @@
import type { Meta, StoryObj } from '@storybook/react';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -1,8 +1,10 @@
import React, { FunctionComponent } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -1,4 +1,5 @@
import type { Meta, StoryObj } from '@storybook/react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -1,10 +1,11 @@
import { Text, TextVariant } from '@metamask/design-system-react';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

@@ -0,0 +1,15 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mdx files similarly to design-system-react

Comment on lines +8 to +10
"paths": {
"@metamask/design-system-react": ["../design-system-react/src"]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added path alias for @metamask/design-system-react to enable direct imports from the design system package.

Before

Screenshot 2024-12-02 at 5 05 58 PM

After

Screenshot 2024-12-02 at 5 06 11 PM

brianacnguyen
brianacnguyen previously approved these changes Dec 3, 2024
@@ -1,3 +1,4 @@
/* eslint-disable import/unambiguous */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disabled a rule to make our eslintrc file more readable

Before

Screenshot 2024-12-03 at 2 20 32 PM

After

Screenshot 2024-12-03 at 2 20 51 PM

@@ -0,0 +1,19 @@
module.exports = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added .eslintrc.js to support browser environment in React package

This ESLint configuration extends the root config but adds browser-specific settings. The root ESLint config is Node.js-focused, so this configuration:

  • Enables browser environment
  • Disables Node.js environment
  • Turns off no-restricted-globals rule to allow browser globals
  • Adds TypeScript configuration for .ts/.tsx files

rules: {
'no-restricted-globals': 'off',
},
ignorePatterns: ['scripts/create-component/ComponentName/**'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring component template files

@brianacnguyen brianacnguyen merged commit 1b26fe5 into main Dec 3, 2024
29 checks passed
@brianacnguyen brianacnguyen deleted the fix/linting-updates branch December 3, 2024 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants